Skip to content

perf(storer): remove per-chunk index lookups - #5615

Closed
gacevicljubisa wants to merge 5 commits into
masterfrom
perf/sample-02-locating-chunkstore
Closed

gacevicljubisa wants to merge 5 commits into
masterfrom
perf/sample-02-locating-chunkstore

Conversation

@gacevicljubisa

Copy link
Copy Markdown
Member

Checklist

  • I have read the coding guide.
  • My change requires a documentation update, and I have done it.
  • I have added tests to cover my changes.
  • I have filled out the description and linked the related issues.

Description

Makes ReserveSample substantially cheaper:

  • chunk data is read into a per-worker buffer instead of a fresh allocation per chunk
  • ChunkBinItem carries a sharky location hint, so the per-chunk retrieval-index lookup disappears; reads fall back to the index when the hint is absent or stale
  • migration step 8 backfills the hint for reserves written before the field existed (without it the hint is inert on an upgraded node)

Measured

Testnet, 2.49M chunk reserve, radius 2, no SIMD, identical config, both nodes sampled simultaneously via /rchash. Median of 3 runs.

per chunk master this PR Δ
ReserveSample allocations 146.8 69.9 −52.6%
chunk_load 9.89 ms 0.48 ms −95%
transformedAddress (control — unchanged code) 67.2 66.6 −1%
sample duration 701 s 227 s −67%

Open API Spec Version Changes (if applicable)

Motivation and Context (Optional)

Related Issue (Optional)

Screenshots (if appropriate):

AI Disclosure

  • This PR contains code that has been generated by an LLM.
  • I have reviewed the AI generated code thoroughly.
  • I possess the technical expertise to responsibly review the code generated in this PR.

gacevicljubisa and others added 5 commits September 28, 2026 12:33
…e sampling

Accelerate ReserveSample by bypassing LevelDB index lookups through
opaque storage.ChunkLocation hints without breaking clean storage
abstractions:

- Introduce storage.ChunkLocation and capability interfaces
  (LocatingPutter, LocatingReplacer, LocatingGetterInto) in pkg/storage
- Support dual-format backward-compatible ChunkBinItem deserialization
  (106 bytes legacy, 114 bytes with location) avoiding database migrations
- Encapsulate thread-safe LocationGuard inside internal/chunkstore to
  prevent use-after-free race conditions when Sharky reuses freed slots
- Use GetIntoLoc in reserve sampler workers with automatic fallback to
  standard GetInto for legacy or guarded locations
…ion hints

Ensure GetIntoLoc defaults to fail-safe behavior by falling back to standard
verified GetInto whenever no sampling session is active:

- Add LocationGuard.SessionActive() method
- Check !guard.SessionActive() in GetIntoLoc before using sharky location
- Update tests to verify fallback when inactive and direct read when active
Align execution order in ReplaceLoc with Delete to enforce the principle
of marking resources in LocationGuard prior to releasing them in Sharky:

- Move guard.MarkFreed before sh.Release in ReplaceLoc
- Add test coverage verifying that old locations trigger safe indexStore
  fallback after ReplaceLoc
…ore the field

The location hint added to ChunkBinItem only lands on entries that are
written after the upgrade. Everything already in the reserve stays in the
106-byte legacy layout, Unmarshal leaves its Location zero, and GetIntoLoc
falls back to the retrieval-index lookup for every one of those chunks. The
dual-format Unmarshal makes the old records readable; nothing populates
them, so on an upgraded node the hint is inert until the reserve turns over
on its own, which takes days or never.

Measured on a 2.41M chunk testnet reserve: GetIntoLoc fell back on 100% of
chunks and leveldbstore.Get stayed at 2731 B/chunk, unchanged from the
branch without the hint.

Add migration step 8, which walks the bin index, resolves each address in
the retrieval index once, and writes the location back.

Entries are flushed in windows rather than collected up front: the reserve
holds millions of items and ReserveRepairer's collect-everything approach
would cost hundreds of megabytes while the node is still starting. Since
Location lives in the value and the key stays (Bin, BinID), rewriting does
not disturb the iteration it runs inside. Each window is spread over
NumCPU workers because the per-entry retrieval-index lookup is a random
read and would otherwise leave the disk idle.

The step is idempotent. Entries that already carry a location are skipped,
and entries whose chunk is gone from the chunkstore keep a zero location so
the sampler keeps falling back for them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018s5GBDGHX6Nop4RxHhsCr2
@gacevicljubisa
gacevicljubisa force-pushed the perf/sample-02-locating-chunkstore branch from 034f242 to 5b0da76 Compare September 28, 2026 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant